We should not do validation of the number of tokens per player when starting the game. This validation and error raising should happen when the server receives the set_vetoes message. This is because as it is now a player could send the server more than the max amount of vetoes and then cause a game to never be able to launch.

Similarly here we are throwing an exception based on user input so a user could craft a veto selection that could cause the majority of games to fail to start. It would be better to have some default value rather than throwing an exception.

you can use *_ instead of 3 _ to unpack unneeded variables

why is it suddenly in camelCase?

calculate_dynamic_tokens_per_map expects list[int], but vetoesMap.values() are actually not a list. if you want to annotate dict's values, you can use ValuesView from collections.abc

good notice
i'll replace calculate_dynamic_tokens_per_map tokens type with Iterable[int]

are you trying to achieve sum(weights) = M?

calculate_dynamic_tokens_per_map finds the minimal possible max_tokens_per_map while still respecting M

but when do you decide that max_tokens_per_map can't be reduced anymore?

Its just some math
it solves series of equations, if vetoes set correctly, one of them is guaranteed to be correct answer
if vetoes set incorrectly, it will return 0

i'll prepare some explanation how math works

why are all of those arguments not inside MapPool 🤔 ?

because its not in the mapPool in the database, but in matchmaker_queue_map_pool (queue bracket), so should be consistent between repos
and yea its that way in db due to some reasons which were discussed long ago (f.e. u can use same pool for multiple brackets)

map pool https://faforever.github.io/db/tables/map_pool.html
mm queue bracket https://faforever.github.io/db/tables/matchmaker_queue_map_pool.html

you could create a container (NamedTuple) for this tuple[int], int, int structure and avoid those long type hints and maybe call it MapPoolVeto

also, if you put tokens and ratings information in MapPool (in database they are in the same table, why are they separated here?) you won't even need this additional structure and you will be able to pass MapPool and use its properties

finally, this can also possibly remove get_pools_veto_data method since it just rearranges MapPool's internals

vetoes are not in map pool in database https://faforever.github.io/db/tables/map_pool.html
vetoes are here: https://faforever.github.io/db/tables/matchmaker_queue_map_pool.html
the same place where min and max rating, outside of map pool itself

also i am not sure wdym removing get_pools_veto_data
it gives only necessary data and readable name for it, allows to avoid any code duplication and keeps the code more clean
i am using it in multiple places so if we remove it code will become just worse

Oh, i see

What i mean is that now matchmaker_queue_map_pool has additional 3 new fields and it may be viable to create corresponding data container (see, for example, FeaturedMod).
This way you preserve readable names and also make it easier to understand what's passed to other functions

For example, in my opinion

async def update_vetoes(self, list[MatchmakerQueueMapPool], current: dict | None) -> None

is easier to follow than to find calls to update_vetoes and discover what's inside of list[tuple[list[int], int, int]]

also,

map_pools: Iterable[MatchmakerQueueMapPool]

is easier to understand than

map_pools: Iterable[tuple[MapPool, Optional[int], Optional[int], int, int, float]] = ()

where MatchmakerQueueMapPool is

class MatchmakerQueueMapPool(NamedTuple):
    map_pool: MapPool
    min_rating: int | None
    max_rating: int | None
    veto_tokens_per_player: int
    max_tokens_per_map: int
    minimum_maps_after_veto: float

finally, this way long unpacking can be avoided and instead of

for pool, *_, veto_tokens_per_player, max_tokens_per_map, _ in queue.map_pools.values():

you can easily access named attributes

for pool in queue.map_pools.values():
    pool.veto_tokens_per_player...

well, yea, but this going into refactoring existing code zone rather than implementing new features so i was not sure should i do it here or not

Using and changing to a NamedTuple should be fine and probably preferred as Gatsik says

cannot put named tuple MatchmakerQueueMapPool right inside types.py due to circular dependencies happening (map_pool.py uses types and types uses map_pool.py in this case)
is it ok no place it right inside map_pool.py?

don't you feel like this line is a bit long?)

long lines for long PP

Maybe the explanation could be shorter

Lets denote max_tokens_per_map = $d$ and len(tokens) = $n$. You are trying to achieve
$\sum_{i=1}^{n}(1 - \frac{a_{i}}{d}) = M$, which means $d = \frac{\sum_{i=1}^{n}a_{i}}{n - M}$

If some $a_{i} > d$ then $1 - \frac{a_{i}}{d} < 0$, which results in contributing nothing due to $weight = \max(0, 1 - \frac{a_{i}}{d})$ . Will excluding those ${a_{i}}$ from $tokens$ decrease $d$?

Given sorted array, excluding the last element ${a_{n}}$ from $tokens$ gives new $d_{1} = \frac{\sum_{i=1}^{n-1}a_{i}}{n - M - 1}$.
This excluding won't help if $d_{1} > d$

Lets denote $\sum_{i=1}^{n-1}a_{i} = S$, then $d < d_{1}$ is achieved when $\frac{S + a_{n}}{n - M} < \frac{S}{n - M - 1}$, which means $a_{n} < \frac{S + a_{n}}{n - M}$

But $S + a_{n} = \sum_{i=1}^{n}a_{i}$, therefore $a_{n} < \frac{\sum_{i=1}^{n}a_{i}}{n - M}$ ⇒ $a_{n} < d$

This means, that we can stop trying to decrease $d$ when $\max(tokens) < d$

And you can describte the algorithm as easy as:

• Find $d = \frac{\sum_{i=1}^{n}a_{i}}{n - M}$, if $d > \max(tokens)$ then $d$ is found. otherwise exclude all $a_{i} \geq d$. Repeat while $n > M$

(And of course handle special case when $d = 0$, which means $d$ can be anything and you've chosen it to be $1$)

maybe put docstring inside the function definition?

I still don't really understand why you need MatchmakerQueueMapPoolVetoData. Why don't you just initialize MatchmakerQueueMapPool with correct vetoes setup and use it?

atleast to avoid having any possible issues with queue.map_pools.clear() in update_data
theoretically user can change vetoes mid-queues-update, then server will delete all user's vetoes and he will need to apply all of them again which is very bad

Askaholic said "Technically we do run the matchmaking algorithm in a separate thread because it is the one piece of the code that has the potential to block the rest of the server if there are a lot of players in queue."

So i prefer to have robust solution here

I don't think that statement is relevant to vetos because the 'chose_map' stage happens after the matchmaking algorithm has finished. But you should probably set it up in such a way that vetos are locked in the moment a match is found like we do with factions in the PlayerParty.on_matched function. I think keeping the behavior for choosing vetos and factions consistent makes the most sense.

I don't think that statement is relevant to vetos because the 'chose_map' stage happens after the matchmaking algorithm has finished. But you should probably set it up in such a way that vetos are locked in the moment a match is found like we do with factions in the PlayerParty.on_matched function. I think keeping the behavior for choosing vetos and factions consistent makes the most sense.

its not about chose_map, it about command_set_player_vetoes in lobbyconection.py
it uses matchmaker map pool data and this is async function, and i am not 100% sure how this works here

also whats the point of locking vetoes when match found? what can possibly go wrong if we dont do that?

Maybe use Optional[int], because looks like server hasn't adopted this syntax yet